Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Module.delegate #2275

Closed
wants to merge 1 commit into from
Closed

Refactor Module.delegate #2275

wants to merge 1 commit into from

Conversation

bogdan
Copy link
Contributor

@bogdan bogdan commented Jul 26, 2011

Rewrite code to have minimum amount of eval.

Properly namespaced the module in the pull request.
Original pull resquest #2255.

@bogdan
Refactor Module.delegate
f6cfe0f 
Rewrite code to have minimum amount of eval
@dasch
Copy link
Contributor

dasch commented Jul 26, 2011

Have you benchmarked the impact this will have on performance? It'll add an extra level of indirection to all delegate calls.

@bogdan
Copy link
Contributor Author

bogdan commented Jul 27, 2011

Here is my test file https://gist.github.com/1108920

The first benchmark is to test intialization.
The second benchmark is to test execution

Here is what I get on 1.8.7:

Rehearsal ------------------------------------
   0.060000   0.010000   0.070000 (  0.062110)
   0.040000   0.000000   0.040000 (  0.045726)
--------------------------- total: 0.110000sec

       user     system      total        real
   0.060000   0.000000   0.060000 (  0.070937)
   0.050000   0.000000   0.050000 (  0.049464)
Rehearsal ------------------------------------
   0.010000   0.000000   0.010000 (  0.014390)
   0.010000   0.000000   0.010000 (  0.007766)
--------------------------- total: 0.020000sec

       user     system      total        real
   0.000000   0.000000   0.000000 (  0.004052)
   0.000000   0.000000   0.000000 (  0.005466)

And for 1.9.2

Rehearsal ------------------------------------
   0.090000   0.000000   0.090000 (  0.099133)
   0.090000   0.000000   0.090000 (  0.083098)
--------------------------- total: 0.180000sec

       user     system      total        real
   0.120000   0.010000   0.130000 (  0.132569)
   0.090000   0.000000   0.090000 (  0.096892)
Rehearsal ------------------------------------
   0.010000   0.000000   0.010000 (  0.007009)
   0.010000   0.000000   0.010000 (  0.008096)
--------------------------- total: 0.020000sec

       user     system      total        real
   0.010000   0.000000   0.010000 (  0.002482)
   0.000000   0.000000   0.000000 (  0.002776)

jeremy
jeremy reviewed Oct 9, 2011
View reviewed changes
activesupport/lib/active_support/core_ext/module/delegation.rb
target.__send__(method, *args, &block)
rescue NoMethodError
if target.nil?
return nil if allow_nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be clearer to have a separate method that swallows nils, rather than passing allow_nil and conditionally handling it here.

@jeremy
Copy link
Member

jeremy commented Oct 9, 2011

Could you rebase against master?

@bogdan
Copy link
Contributor Author

bogdan commented Oct 11, 2011

Closed. See #3290 instead.

@bogdan bogdan closed this Oct 11, 2011
@bogdan bogdan mentioned this pull request Oct 12, 2011
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bogdan @dasch @jeremy